serviceability-instruction/sdk-rs: carry the IP ownership proof and Ed25519 instruction - #4224
Conversation
juan-malbeclabs
left a comment
juan-malbeclabs
left a comment
There was a problem hiding this comment.
Reviewed the RFC-27 client side. The mechanism is correct: I re-derived the Ed25519 precompile layout against solana-ed25519-program-3.0.0 (DATA_START=16, key→sig→msg order, the u16::MAX sentinel) and it agrees with check_ed25519_instruction's reconstruction; the sysvar scan is position-independent and bounds-checked; the epoch window reads correctly at epoch 0; and proof_owner lines up with create_user_core's effective_owner on both paths (CreateUser passes owner_override: None, so the payer is right; CreateSubscribeUser derives it from args.owner, so accesspass_payer is right). No wrong conditions, off-by-ones, dropped errors, or broken callers.
Two notes, both on the local pre-send gate this PR introduces, inline.
Unrelated to the diff but newly relevant: client.rs:244 maps InstructionError::Custom(n) to DoubleZeroError::from(n) and discards the instruction index. That was safe while every instruction was serviceability's — now that transactions carry a precompile instruction whose PrecompileError lives in the same Custom space, InvalidSignature (2) would print as InvalidExchangePubkey. Unreachable while skip_preflight is hardcoded true, so a comment or a cheap index guard is enough.
Verified locally: cargo test -p doublezero-serviceability-instruction (81 pass), cargo test -p doublezero_sdk (189 pass, including all 7 new RFC-27 command tests), rfc26_builders_test test_builder_user_creation_with_ip_proof, and cargo check --workspace --all-targets (only the pre-existing, unrelated doublezero-geolocation::entrypoint test-target failures).
f83a28f to
f0226ab
Compare
juan-malbeclabs
left a comment
juan-malbeclabs
left a comment
There was a problem hiding this comment.
Re-reviewed at f0226ab4. Both inline notes and the Custom(n) point from my review body are resolved, and I re-checked the rebase delta on top of #4120.
Signature check (sdk/rs/src/commands/user/mod.rs:84) — verify(proof, &verifier) now runs after the GlobalState lookup and before the Ed25519 instruction is built, so a proof signed by a rotated key fails locally with a named error instead of the invisible confirmation timeout. The signer feature is enabled on the sdk/rs dependency only; the crate's default build stays borsh + solana-program, so the BPF graph is unaffected. Both proof_for helpers now sign for real, which means the happy-path tests exercise a signature that actually verifies rather than [5u8; 64], and test_commands_user_create_with_ip_proof_rejects_a_rotated_verifier_key covers the rotation case directly.
Version check (mod.rs:49) — is_supported_version runs first, matching the program's order in ip_proof.rs:125, and the epoch window is still correctly left to the program with the doc comment explaining why.
Custom(n) mapping (client.rs:250) — the guard on transaction.message.program_id(index) is the better fix over the comment I suggested. It reads the legacy message the send path actually builds, an out-of-range index falls through to the raw error, and it is strictly more correct for any non-serviceability instruction, not just the precompile.
Transaction-size guard (create_subscribe.rs:154-180) — assembling the instruction list before the measurement is right. The Ed25519 instruction carries ~170 bytes of data plus the sysvar and precompile account keys, which the old [cu_limit, heap_frame, ix] message missed entirely; the group cap is now judged against what goes on the wire. The accesspass_owner parameter on expect_create_lookups is a faithful adaptation — the --owner test seeds the pass under the owner and signs the proof for the owner, which is what create_user_core's effective_owner resolves to on that path.
ip_proof.rs in the instruction crate is byte-identical to what I reviewed pre-rebase; the only other delta is the test-only &[] for the new extra_mgroup_pks parameter.
Two non-blocking notes, neither worth another round:
match instructions.len() { 1 => ..., _ => ... }increate_subscribe.rswould read more directly as a match onself.ip_proof, since that is what actually decides the shape.- Every caller still passes
ip_proof: None, so RFC-27 is not reachable end-to-end yet. Expected — the issuing side lands separately — just flagging that nothing exercises this outside tests today.
Approving.
…d25519 instruction Resolves #4200. Part of RFC-27; tracker #4194. The program validates an optional IpOwnershipProof as of #4211, but nothing client-side could produce a transaction carrying one. This adds the two pieces a caller needs: the native Ed25519SigVerify instruction the program introspects the Instructions sysvar to find, and the Rust SDK plumbing to send it alongside the creation it authorizes. - crates/doublezero-serviceability-instruction gains an ip_proof module: ed25519_verification_instruction lays out the precompile instruction for a proof, and with_ed25519_verification pairs it ahead of the create instruction. The offset layout comes from solana_ed25519_program rather than being written out, because the program rejects any instruction whose offsets name another instruction or run past the end of its data. - The builders keep their signatures: #4211 already put ip_proof in the args and made them append the Instructions sysvar from it, so the proof travels in one place rather than two that can disagree. - DoubleZeroClient gains send_instructions for a transaction that needs more than one instruction. send_transaction is unchanged. - CreateUserCommand and CreateSubscribeUserCommand take an optional ip_proof. A shared helper resolves the verifier key from GlobalState.ip_verifier_authority_pk, the same place the program reads it, so a caller cannot pair a proof with the wrong key, and refuses a proof naming a different owner, address, or user type before the transaction is paid for. On the owner-override path the proof must name that owner: the program binds it to the user's effective owner, not the payer. Omitting the proof produces the pre-RFC-27 transaction unchanged. - Nothing sets ip_proof yet; the CLI is #4201. Transaction headroom, pinned by tests: with a proof attached CreateUser fits 10 dz_prefix_block accounts and CreateSubscribeUser 8, against 21 and 19 without one. The proof costs about eleven slots — the 111-byte Option<IpOwnershipProof> in the args, a 169-byte Ed25519 instruction, and two more account keys. Devices carry one or two prefixes.
The local pre-send gate mirrored the program's payer, client_ip and user_type comparisons but not its version check, and never checked the proof's signature against the verifier key it reads from GlobalState. A proof signed by a rotated verifier key passed every check, and the Ed25519 instruction was then built with the current key over the stale signature. The precompile rejects that in the leader, and because the send path uses skip_preflight the transaction never lands: no TransactionError, no logs, just a confirmation timeout. That was the one failure in this path with no named error, which is what the local gate exists to prevent. An unsupported version was cheaper but still paid for: the transaction lands and the program returns IpProofVersionUnsupported. Also stop mapping every InstructionError::Custom through DoubleZeroError. Custom numbers belong to whichever program raised them, and these transactions now carry a precompile instruction whose PrecompileError shares that space, so the mapping is guarded by the failing instruction's program id. The SDK enables doublezero-ip-proof's signer feature for the verification half only; it still never issues a proof.
f0226ab to
b57e469
Compare
Resolves #4201. Part of RFC-27; tracker #4194. Stacked on #4224. The SDK can carry an RFC-27 proof as of #4200, but nothing obtained one. This makes `doublezero connect` ask the verification service for a proof and attach it to user creation. - config: NetworkConfig gains ip_verifier_url, overridable by DZ_IP_VERIFIER_URL and by a new --ip-verifier-url on connect. Only localnet has a built-in default, the verifier's own listen address; deployed URLs land with #4199, and until then those environments simply have no verifier. - New ip_proof module in doublezero-daemon-cli: an automock'd IpProofClient trait plus a blocking reqwest implementation. The request is bound to the address the tunnel will use, so a multi-homed host proves the address it actually originates from; on a NATed host that bind fails and the request falls back to the default egress, where the service observes the NAT address the daemon already discovered. - The service's observed address is authoritative. Where it disagrees with what the daemon discovered, connect stops and names both: attaching the proof would guarantee an onchain rejection, and dropping it would bind an address nobody proved. - Every other failure is reported and non-fatal — unconfigured, unreachable, or declined (a CGNAT source, a rate limit) each print the specific reason and continue without a proof. The program is the enforcement point, so this succeeds while require-ip-ownership-proof is clear and fails with a named error once it is set, which is the behavior wanted during rollout. - One proof per invocation: a single connect only ever creates users of one user_type, and the proof binds user_type. The issue asks to fail when an explicit --client-ip disagrees with the service. That flag is deprecated and ignored on the CLI, so the disagreement that can actually happen is between the daemon's discovered address and the service's observed one; that is what is checked.
Resolves #4201. Part of RFC-27; tracker #4194. Stacked on #4224. The SDK can carry an RFC-27 proof as of #4200, but nothing obtained one. This makes `doublezero connect` ask the verification service for a proof and attach it to user creation. - config: NetworkConfig gains ip_verifier_url, overridable by DZ_IP_VERIFIER_URL and by a new --ip-verifier-url on connect. Only localnet has a built-in default, the verifier's own listen address; deployed URLs land with #4199, and until then those environments simply have no verifier. - New ip_proof module in doublezero-daemon-cli: an automock'd IpProofClient trait plus a blocking reqwest implementation. The request is bound to the address the tunnel will use, so a multi-homed host proves the address it actually originates from; on a NATed host that bind fails and the request falls back to the default egress, where the service observes the NAT address the daemon already discovered. - The service's observed address is authoritative. Where it disagrees with what the daemon discovered, connect stops and names both: attaching the proof would guarantee an onchain rejection, and dropping it would bind an address nobody proved. - Every other failure is reported and non-fatal — unconfigured, unreachable, or declined (a CGNAT source, a rate limit) each print the specific reason and continue without a proof. The program is the enforcement point, so this succeeds while require-ip-ownership-proof is clear and fails with a named error once it is set, which is the behavior wanted during rollout. - One proof per invocation: a single connect only ever creates users of one user_type, and the proof binds user_type. The issue asks to fail when an explicit --client-ip disagrees with the service. That flag is deprecated and ignored on the CLI, so the disagreement that can actually happen is between the daemon's discovered address and the service's observed one; that is what is checked.
2 participants
Resolves #4200. Part of RFC-27 (rfcs/rfc27-ip-verification.md); tracker #4194.
Summary of Changes
IpOwnershipProofas of serviceability: validate IpOwnershipProof via the Ed25519 precompile #4211, but nothing client-side could build a transaction carrying one. This adds the two missing pieces: the nativeEd25519SigVerifyinstruction the program introspects the Instructions sysvar to find, and the Rust SDK plumbing to send it alongside the creation it authorizes.ip_proofmodule indoublezero-serviceability-instruction:ed25519_verification_instructionlays out the precompile instruction for a proof, andwith_ed25519_verificationpairs it ahead of the create instruction. The offset layout comes fromsolana_ed25519_programrather than being written out here — the program rejects any instruction whose offsets name a different instruction or run past the end of its data, so a hand-rolled header is a silent way to build a transaction that can never land.Option<IpOwnershipProof>, but serviceability: validate IpOwnershipProof via the Ed25519 precompile #4211 already putip_proofinUserCreateArgs/UserCreateSubscribeArgsand made both builders derive the Instructions sysvar append from it. Adding a positional parameter would give the proof two homes that can disagree, so it stays in the args.DoubleZeroClientgainssend_instructionsfor a transaction needing more than one instruction.send_transactionis unchanged, so its 223 call sites are untouched.CreateUserCommandandCreateSubscribeUserCommandtake an optionalip_proof. A shared helper resolves the verifier key fromGlobalState.ip_verifier_authority_pk— the same place the program reads it — so a caller cannot pair a proof with the wrong key, and refuses a proof naming a different owner, address, or user type before the transaction is paid for. The epoch window is deliberately left to the program: the ledger's current epoch is its to judge.--owneroverride path the proof must name that owner, not the payer.create_user_corebinds the proof to the user's effective owner, which differs from the payer on the foundation-allowlist path.Noneproduces the pre-RFC-27 transaction byte for byte. Nothing setsip_proofyet; the CLI obtaining a proof duringconnectis cli: obtain an IP ownership proof during connect and attach it to user creation #4201.Transaction size
The issue asks for the headroom at realistic
dz_prefix_count, and two tests pin it rather than leaving it to a comment. With a proof attached,CreateUserfits 10dz_prefix_blockaccounts andCreateSubscribeUser8, against 21 and 19 without one — the proof costs about eleven slots: the 111-byteOption<IpOwnershipProof>in the args, a 169-byte Ed25519 instruction, and two more account keys (the Instructions sysvar and the Ed25519 program). Devices carry one or two prefixes, so the margin is large either way, but a future field cannot quietly eat the rest of it without failing these tests.Diff Breakdown
The core-logic files carry 507 lines of inline
#[cfg(test)]tests of their own, leaving about 200 lines of new logic — two builders, one shared SDK helper, and one trait method.Key files (click to expand)
crates/doublezero-serviceability-instruction/src/ip_proof.rs— new module: the Ed25519 precompile instruction for a proof, and the ordered pairsmartcontract/sdk/rs/src/commands/user/mod.rs— the sharedinstructions_with_ip_proof: verifier-key lookup plus the three field checks the program also makes, in one place so the two commands cannot driftsmartcontract/sdk/rs/src/commands/user/create.rs—ip_prooffield, and the two-instruction send when one is suppliedsmartcontract/sdk/rs/src/commands/user/create_subscribe.rs— the same, binding the proof toaccesspass_payer(the effective owner) rather than the payersmartcontract/sdk/rs/src/client.rs— the inner send path now takes aVec<Instruction>smartcontract/sdk/rs/src/doublezeroclient.rs—send_instructionson the traitcrates/doublezero-serviceability-instruction/src/user.rs— the two transaction-size headroom testssmartcontract/programs/doublezero-serviceability/tests/rfc26_builders_test.rs— end-to-end acceptance against the real programTesting Verification
test_builder_user_creation_with_ip_proofruns both builders against the in-process program withrequire-ip-ownership-proofset and a real verifier key in global state, so each creation only lands if the builder-assembled Ed25519 instruction actually validates — the acceptance criterion. Both users endActivated, and the multicast one carries its subscription. This is the first test in the repo that submits a precompile instruction produced by production code rather than a test helper.test_builder_create_subscribe_useris theNonecase end to end, and builder-level tests assert that aNoneproof leaves both account lists byte-identical to the pre-RFC-27 layout.u16::MAXsentinel, and the key / signature / message slices at the declared offsets. A separate test bends the proof's epoch and asserts the covered message moves with it, so the builder can never sign for a message the program will not reconstruct.--ownerpath — a proof bound to the payer instead of the owner.dz_prefix_countwith and without a proof, for both instructions.user_ip_proof_test(34),doublezero-daemon-cli(180), anddoublezero-serviceability-cli(419) all still pass.make generate-fixturesproduces no.bin/.jsonchange, confirming the no-proof wire shape is untouched.